Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add SNMP Monitor #4717

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

feat: Add SNMP Monitor #4717

wants to merge 48 commits into from

Conversation

mattv8
Copy link

@mattv8 mattv8 commented Apr 27, 2024

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

This PR introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol). The following changes have been made:

  • Added new fields for SNMP configuration, including community string, OID (Object Identifier), SNMP version, control value, and condition.
  • Updates the database schema to include SNMP-related columns.
  • Implemented SNMP check logic to determine device status based on SNMP values and configured conditions.
  • Utilizes net-snmp.

Resolves #1675

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

image

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).
@mattv8 mattv8 mentioned this pull request Apr 27, 2024
1 task
@mattv8 mattv8 changed the title feat: Add SNMP Monitor feat: Add SNMP Monitor (WIP) Apr 27, 2024
net-snmp over snmp-native is:
-more robust
-more popular
-better documented
-supports v3
Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.
@mattv8

This comment has been minimized.

@CommanderStorm

This comment has been minimized.

Thanks for pointing it out @CommanderStorm!
@mattv8

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

@mattv8

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

DB must allow nulls otherwise this will break other monitors.
knex requires down function
Updates appropriate values async when editing the SNMP monitor
@mattv8 mattv8 changed the title feat: Add SNMP Monitor (WIP) feat: Add SNMP Monitor Apr 30, 2024
@mattv8 mattv8 marked this pull request as ready for review April 30, 2024 21:46
@mattv8
Copy link
Author

mattv8 commented Apr 30, 2024

@CommanderStorm I'm ready for review. It is not passing ES Lint, although when I run npm run lint:prod --fix there are some changes to server/model/monitor.js that are unrelated to this PR. I'm unsure as how to proceed on the linting side of things.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at https://github.com/louislam/uptime-kuma/pull/4717/files or https://github.com/louislam/uptime-kuma/actions/runs/8902151438/job/24447507847?pr=4717 and fix the linting mistakes.
I don't know why your local environment is producing different linting results.

I am certain that a review at this time will miss too many things and will thus require another review cycle. Nevertheless, I have attached a few comments.

src/pages/.EditMonitor.vue.swp Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
db/knex_migrations/2024-04-26-0000-snmp-monitor.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
Comment on lines 61 to 90
switch (monitor.snmpCondition) {
case '>':
heartbeat.status = numericValue > monitor.snmpControlValue ? UP : DOWN;
break;
case '>=':
heartbeat.status = numericValue >= monitor.snmpControlValue ? UP : DOWN;
break;
case '<':
heartbeat.status = numericValue < monitor.snmpControlValue ? UP : DOWN;
break;
case '<=':
heartbeat.status = numericValue <= monitor.snmpControlValue ? UP : DOWN;
break;
case '==':
if (!isNaN(value) && !isNaN(monitor.snmpControlValue)) {
// Both values are numeric, parse them as numbers
heartbeat.status = parseFloat(value) === parseFloat(monitor.snmpControlValue) ? UP : DOWN;
} else {
// At least one of the values is not numeric, compare them as strings
heartbeat.status = value.toString() === monitor.snmpControlValue.toString() ? UP : DOWN;
}
break;
case 'contains':
heartbeat.status = stringValue.includes(monitor.snmpControlValue) ? UP : DOWN;
break;
default:
heartbeat.status = DOWN;
heartbeat.msg = `Invalid condition: ${monitor.snmpCondition}`;
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About this part @chakflying has commented in #1675 (comment)

Sounds pretty good, but just in case you didn't know, you should take a look at how the json-query monitor works.

Ideally in the long term, we would want all value comparisons to work with the jsonata syntax, and reuse the database fields for better compatibility (see #3919). I don't think it's worth implementing custom value comparison functionality just for this monitor.

=> This needs to be compatible with #4617 and #3919

@chakflying what do you think is the best course forward?

Copy link
Author

@mattv8 mattv8 May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chakflying & @CommanderStorm I propose waiting until after #4617 is committed and I will re-factor & maintain the SNMP monitor after the fact.

server/monitor-types/snmp.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
server/monitor-types/snmp.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
@CommanderStorm
Copy link
Collaborator

@chakflying (sorry to ping you, hope this is fine ^^)
What do you think about the approach used in this PR?
Is this acceptable or should we "force" the users to use json-query expressions publicly (with a longer helptext how the expressuions above work) as well?

@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 23, 2024
@chakflying
Copy link
Collaborator

  • The idea behind my suggestion of using json-query for value comparisons, is that it can provide the maximum freedom for users to process and transform the input data before comparison.
  • If all we are using the library for is running a few preset queries, we are not providing that freedom to the user. Then we may as well have not used the library.
  • However, I also see that there is benefit in being able to set a custom condition/comparison operator other than ==, since it can simplify the expression in complicated cases. (Basically the idea in feat: extend monitor JSON data evaluation operators #4617)

My ideal implementation would be like this:

  • We reuse existing database columns json_path and expected_value, instead of creating new one snmpControlValue
  • We name the new column for the comparison operator json_path_operator instead of the current snmp_condition (as it was in feat: extend monitor JSON data evaluation operators #4617)
  • Optionally, we can default json_path to $.value for people who do not require any transformation.
  • When the monitor runs its check,
    • we obtain the value from SNMP as it's currently done: const value = varbinds[0].value;
    • we evaluate this value according to user's inputted json_path:
         let expression = jsonata(this.jsonPath);
         let result = await expression.evaluate(value);
      
    • We compare result with expected_value using the user's chosen operator (==, >=, etc)
    • This comparison produces a true/false result which will determine the monitor status

This implementation would be maximally compatible with the existing json-query monitor, and have the benefit that the user has much more freedom, e.g. can quickly check if the SNMP value is odd by doing $.value % 2 != 0 (I think).

I really appreciate the work that you have put into this feature, it looks great already. I don't want this to be like "great, but here's more work for you". It's difficult to reach an ideal solution when we all have different ideas of how it would look like. I'm also not in a good position to comment since I don't have an SNMP device to test. Feel free to discuss if needed.

@chakflying
Copy link
Collaborator

BTW, if we are reusing radiusPassword for the storing the community string, then I guess a new database column for that is also not needed anymore?

@mattv8
Copy link
Author

mattv8 commented May 28, 2024

Hey @chakflying I appreciate your thoughts on all this! Currently on mobile and on vacation, so forgive this for being brief. I'll jump back into this in a week or so when I'm back in town and solid internet.

Yes, what you're describing I feel is absolutely workable. I think we need to walk the line between absolute freedom of an open text input for json-queries and the simplicity of the comparison drop down. Let me work through this and see what I can come up with!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again needs:review this PR needs a review by maintainers or other community members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNMP Value Retrieval
3 participants